fix(eval): build the vector index so semantic benchmarks can report a number; stop tests writing to $HOME - #710
Conversation
… number `run_eval` built the graph and called `run_post_processing(store)`, but that step's embedding refresh is a refresh, not a bootstrap: `refresh_embeddings()` returns early on a graph with no existing vectors, deliberately, so that no build path can silently load a model or incur API cost. Nothing else in the eval framework populated the index. So every benchmark that puts a natural-language question through `hybrid_search` — `agent_baseline`, `search_quality`, `multi_hop_retrieval` — fell through to FTS5, which scores a full sentence against no matching document and returns nothing. Every row came back `status="no_graph_results"`, and `aggregate()` excludes those rows, so the run reported `ok_rows: 0` / `median: None` instead of an error. Reproduced on the shipped fastapi config: all three `agent_questions` returned 0 hits against a healthy graph (6287 nodes, FTS5 populated with 6287 rows). After building the index, the same three questions return 28.1x / 82.4x / 68.0x. That is why no `agent_baseline` CSVs exist under `evaluate/results/` even though README.md cites their path. Changes: - `run_eval(embed=..., embedding_provider=..., embedding_model=...)` with a new `_build_embedding_index()` that mirrors `tools.docs.embed_graph` but reuses the runner's already-open store instead of opening a second connection to the same database. Default off, so the cost invariant that `refresh_embeddings` protects is unchanged. - `_warn_if_semantic_index_missing()` runs before the benchmarks and names the affected ones, so the failure announces itself instead of arriving as an empty aggregate. - `agent_baseline.aggregate()` reports `no_graph_results_rows` and `no_baseline_match_rows`. Excluded rows are now counted, not just dropped: "no result" and "every query failed" had the same signature before. - `eval --embed` / `--embed-provider` / `--embed-model` on the CLI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Running `pytest` left entries like this in the developer's own
`~/.code-review-graph/registry.json`:
{"path": "/tmp/pytest-of-<user>/pytest-0/test_registry_data_dir_overrid0/project",
"data_dir": "/tmp/pytest-of-<user>/pytest-0/.../external"}
Two routes reached the real home directory:
- `Registry()` defaults to `~/.code-review-graph/registry.json`, and
`incremental.get_data_dir()` constructs one internally. Tests covering
data-dir resolution therefore both read and wrote the registry of whoever
ran the suite. Besides the pollution, that made those tests depend on
machine state: a developer with a registered repo could get a different
result from one without.
- `daemon.py` built `CONFIG_PATH`, `PID_PATH`, `STATE_PATH` and
`DaemonConfig.log_dir` from `Path.home()` as import-time constants.
An import-time constant cannot be redirected after the fact, which is the
core of the problem: by the time a fixture could set anything, the value is
already frozen. So the paths resolve per call now.
- `constants.crg_home()` reads `$CRG_HOME`, falling back to
`~/.code-review-graph`. Same convention as the existing `CRG_DATA_DIR`.
- `registry.default_registry_path()` and the four `daemon.default_*()`
helpers route through it.
- `daemon.CONFIG_PATH` / `PID_PATH` / `STATE_PATH` keep working through a
PEP 562 module `__getattr__`, so nothing that imported them breaks.
- `tests/conftest.py` points `$CRG_HOME` at a tmp directory for every test.
Autouse and unconditional — an opt-in fixture stops protecting a test the
day someone forgets to request it.
Verified by deleting `~/.code-review-graph` and running the full suite: it
is no longer recreated. Ten regression tests cover both modules, including
that the values are not frozen at import and that the legacy attribute names
still resolve.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jordandunmire97-ai
left a comment
There was a problem hiding this comment.
Idealization of what is prospering
|
The vector bootstrap and test-home fixes are valid, but end-to-end validation found two remaining blockers: custom provider/model indexes were built under one identity while all three benchmarks queried the default identity ( |
Preserves both contributor-authored commits from #710, followed by the maintainer provider/model propagation and resource-cleanup fix. Exact head passed 203 focused tests, real multiprocessing parity, Ruff/diff, a full graph build, CodeQL, and all hosted CI jobs including Windows.
|
The corrected integration #757 is now merged at |
Pull Request
Linked issue
Closes #711 —
agent_baseline/search_quality/multi_hop_retrievalalways reportno_graph_results, because the eval framework never builds the vector index.Closes #712 — running the test suite writes pytest temp paths into the developer's real
~/.code-review-graph/registry.json.Both were found while reproducing the README benchmarks, not from a user report.
What & why
Two independent fixes, one commit each.
1. The eval framework never built the vector index
run_evalbuilds the graph and callsrun_post_processing(store), but that step's embedding refresh is a refresh, not a bootstrap:refresh_embeddings()returns early on a graph with no existing vectors — deliberately, so no build path can silently load a model or incur API cost. Nothing else in the eval framework populated the index.So every benchmark that puts a natural-language question through
hybrid_search—agent_baseline,search_quality,multi_hop_retrieval— fell through to FTS5, which scores a full sentence against no matching document and returns nothing. Every row came backstatus="no_graph_results", andaggregate()excludes those rows, so the run reportedok_rows: 0/median: Nonerather than an error.Reproduced on the shipped
fastapiconfig. The graph itself was healthy — 6287 nodes, FTS5 populated with 6287 rows — but all threeagent_questionsreturned 0 hits:Bare identifiers worked fine (
'APIRoute'→ 5 hits), which is what makes the failure quiet: FTS5 is doing its job, it just can't match a sentence.After building the index, the same three questions produce usable rows:
This is likely why no
agent_baselineCSVs exist underevaluate/results/even though README.md cites the pathevaluate/results/<repo>_agent_baseline_*.csvas the realistic-baseline measurement.Changes:
run_eval(embed=..., embedding_provider=..., embedding_model=...), plus_build_embedding_index(), which reads the graph through the runner's already-openGraphStore. Default off, so the cost invariantrefresh_embeddingsprotects is unchanged._warn_if_semantic_index_missing()runs before the benchmarks and names the affected ones, so the failure announces itself instead of arriving as an empty aggregate.agent_baseline.aggregate()reportsno_graph_results_rows/no_baseline_match_rows. Excluded rows are counted, not just dropped — "no result" and "every query failed" had the same signature before.eval --embed/--embed-provider/--embed-model.run_eval's new parameters are optional and trailing, so the signature stays source-compatible.2. The test suite wrote into the developer's home directory
Running
pytestleft entries like this in the real~/.code-review-graph/registry.json:{"path": "/tmp/pytest-of-<user>/pytest-0/test_registry_data_dir_overrid0/project", "data_dir": "/tmp/pytest-of-<user>/pytest-0/.../external"}Two routes reached it:
Registry()defaults to~/.code-review-graph/registry.json, andincremental.get_data_dir()constructs one internally (incremental.py:316). Tests covering data-dir resolution therefore both read and wrote the registry of whoever ran the suite. Besides the pollution, that made those tests machine-dependent: a developer with a registered repo could get a different result from one without.daemon.pybuiltCONFIG_PATH,PID_PATH,STATE_PATHandDaemonConfig.log_dirfromPath.home()as import-time constants.The common root is that an import-time constant cannot be redirected afterwards — by the time a fixture could set anything, the value is frozen. So the paths resolve per call:
constants.crg_home()reads$CRG_HOME, falling back to~/.code-review-graph. Same convention as the existingCRG_DATA_DIR.registry.default_registry_path()and fourdaemon.default_*()helpers route through it.daemon.CONFIG_PATH/PID_PATH/STATE_PATHkeep working via a PEP 562 module__getattr__, with__dir__so they stay visible to introspection. Not covered:import *— the module defines no__all__, and adding one would change what the star exports for every other name.tests/conftest.pypoints$CRG_HOMEat a tmp directory for every test. Autouse and unconditional: an opt-in fixture stops protecting a test the day someone forgets to request it.Scoped deliberately — the editor-integration installers in
skills.pywrite to~/.codex,~/.cursorand~/.config/opencode, which are outside CRG state and whose tests already patchPath.home()themselves.How it was tested
End-to-end, from a cold start with the graph database deleted:
The home-directory fix was verified by deleting
~/.code-review-graphand running the full suite: it is no longer recreated.test_daemon.pywas the file recreating it (bisected).Each new test was checked against a reverted source tree to confirm it actually fails without the fix, rather than passing vacuously.
Note on reproducibility
docs/REPRODUCING.mdstates two runs on different machines produce identical numbers. That holds for the parsing/FTS/graph benchmarks, but the embedding-backed ratios drift slightly between runs on the same machine (82.3 → 82.4, 67.9 → 68.0). Not addressed here — flagging it in case you want the claim narrowed to the deterministic benchmarks.Checklist
uv run pytest tests/ --tb=short -quv run ruff check code_review_graph/uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional